Fix add_spk API #118

rajarshimaitra · 2022-12-28T08:04:17Z

Check if the spk already exists. If yes, returns false, or else add the spk into map and returns true

Fixes #98

Check if the spk already exists. If yes, returns false, or else add the spk into map and returns true

codecov-commenter · 2022-12-28T08:28:37Z

Codecov Report

Merging #118 (46524ad) into master (b6a9886) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #118   +/-   ##
=======================================
  Coverage   59.39%   59.39%           
=======================================
  Files           8        8           
  Lines         266      266           
=======================================
  Hits          158      158           
  Misses        108      108

Impacted Files	Coverage Δ
bdk_chain/src/keychain/keychain_txout_index.rs	`0.00% <ø> (ø)`
bdk_chain/src/lib.rs	`100.00% <ø> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

LLFourn · 2022-12-29T01:25:52Z

Thaks @rajarshimaitra. Shall we also change the name to insert_script_pubkey to be consistent with the other apis?

rajarshimaitra · 2022-12-29T06:52:22Z

Done..

evanlinjin

Concept ACK 6a39119

I like how this is looking.

evanlinjin · 2022-12-30T18:00:10Z

bdk_chain/src/lib.rs

@@ -37,13 +37,15 @@ pub mod collections {
    #![allow(dead_code)]
    pub type HashSet<K> = alloc::collections::BTreeSet<K>;
    pub type HashMap<K, V> = alloc::collections::BTreeMap<K, V>;
+    pub use alloc::collections::btree_map::Entry;


I think we should remove this.

Not sure why though.. We can, but then we need to specify which::Entry at call site and feature gate it.. Not creating feature gated collection imports is what this module was intended to solve.. But maybe I am missing something.. Do you have a better way to call entry without explicit import?

If someone wants to import both bdk_core::collections::Entry (which is only a btree_map::Entry) and std::collections::hash_map::Entry at the same time, it will get messy. I think having to explicitly state which Entry it is, is a good-to-have.

Hmm.. I think I am getting what you are saying.. I tried it out but unfortunately this particular Entry itself will have three variants depending on feature list.. And different parent modules are available with different features.. So there's no way to do it without different feature gating combination at each module we wanna call Entry in.. It seems to me this way of consolidating all required imports is better than having various feature gates in different modules.. If later on we want to use both kinda Entry together we can explicitly import and give different names to them here in collections module. like use::std::collections::btree_map::Entry as StdBtreeEntry or something like that..

We can make hash_brown a non-optional dependency if that helps.

evanlinjin · 2022-12-30T18:00:27Z

bdk_chain/src/lib.rs

    pub use alloc::collections::*;
 }

 // When we have std use `std`'s all collections
 #[cfg(all(feature = "std", not(feature = "hashbrown")))]
 #[doc(hidden)]
 pub mod collections {
+    pub use std::collections::hash_map::Entry;


I also think this should be removed.

evanlinjin · 2022-12-30T18:01:45Z

bdk_chain/src/lib.rs

@@ -55,4 +57,5 @@ pub mod collections {
    pub type HashSet<K> = hashbrown::HashSet<K>;
    pub type HashMap<K, V> = hashbrown::HashMap<K, V>;
    pub use alloc::collections::*;
+    pub use hashbrown::hash_map::Entry;


Suggested change

pub use hashbrown::hash_map::Entry;

pub use hashbrown::hash_map;

I think this would make more sense, then the above two lines can be removed and everything will be consistent.

What exactly are you referring as inconsistent?

I meant that you can remove the two lines above without resulting in inconsistency.

Same thing as here #118

LLFourn

Looks good. Just need to sort out the Entry export thing. I think you can just export the hash_map module from each thing and import it has collections::hash_map::Entry?

bdk_chain/src/spk_txout_index.rs

LLFourn · 2023-01-05T05:43:47Z

bdk_chain/src/lib.rs

@@ -37,13 +37,15 @@ pub mod collections {
    #![allow(dead_code)]
    pub type HashSet<K> = alloc::collections::BTreeSet<K>;
    pub type HashMap<K, V> = alloc::collections::BTreeMap<K, V>;
+    pub use alloc::collections::btree_map::Entry;


We can make hash_brown a non-optional dependency if that helps.

Import `hash_map` module from each category and call `collections::hash_map::Entry` at call site. plus some nit fixes.

rajarshimaitra · 2023-01-06T06:41:55Z

@LLFourn updated as suggestied. Let me know if this is how you meant to import..

We can make hash_brown a non-optional dependency if that helps.

Could be, but what we have currently doesn't seem to be bad either..

LLFourn

ACK 46524ad

evanlinjin

LGTM!

Fix add_spk API

6a39119

Check if the spk already exists. If yes, returns false, or else add the spk into map and returns true

rajarshimaitra force-pushed the add-spk-fix branch from 1a54df7 to 6a39119 Compare December 28, 2022 08:22

rajarshimaitra force-pushed the add-spk-fix branch from fb77897 to f7cc15e Compare December 29, 2022 05:42

Rename add_spk to insert_script_pubkey

6fb1738

rajarshimaitra force-pushed the add-spk-fix branch from f7cc15e to 6fb1738 Compare December 29, 2022 05:54

evanlinjin suggested changes Dec 30, 2022

View reviewed changes

LLFourn reviewed Jan 5, 2023

View reviewed changes

Change hash_map::Entry import.

46524ad

Import `hash_map` module from each category and call `collections::hash_map::Entry` at call site. plus some nit fixes.

LLFourn approved these changes Jan 9, 2023

View reviewed changes

evanlinjin approved these changes Jan 10, 2023

View reviewed changes

LLFourn merged commit e9ac33e into LLFourn:master Jan 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix add_spk API #118

Fix add_spk API #118

rajarshimaitra commented Dec 28, 2022

codecov-commenter commented Dec 28, 2022 •

edited

Loading

LLFourn commented Dec 29, 2022

rajarshimaitra commented Dec 29, 2022

evanlinjin left a comment

evanlinjin Dec 30, 2022

rajarshimaitra Dec 31, 2022

evanlinjin Jan 2, 2023

rajarshimaitra Jan 2, 2023 •

edited

Loading

LLFourn Jan 5, 2023

evanlinjin Dec 30, 2022

evanlinjin Dec 30, 2022

rajarshimaitra Dec 31, 2022

evanlinjin Jan 2, 2023

rajarshimaitra Jan 2, 2023 •

edited

Loading

LLFourn left a comment

LLFourn Jan 5, 2023

rajarshimaitra commented Jan 6, 2023

LLFourn left a comment

evanlinjin left a comment

	pub use hashbrown::hash_map::Entry;
	pub use hashbrown::hash_map;

Fix add_spk API #118

Fix add_spk API #118

Conversation

rajarshimaitra commented Dec 28, 2022

codecov-commenter commented Dec 28, 2022 • edited Loading

Codecov Report

LLFourn commented Dec 29, 2022

rajarshimaitra commented Dec 29, 2022

evanlinjin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajarshimaitra Jan 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajarshimaitra Jan 2, 2023 • edited Loading

Choose a reason for hiding this comment

LLFourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajarshimaitra commented Jan 6, 2023

LLFourn left a comment

Choose a reason for hiding this comment

evanlinjin left a comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 28, 2022 •

edited

Loading

rajarshimaitra Jan 2, 2023 •

edited

Loading

rajarshimaitra Jan 2, 2023 •

edited

Loading